Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

Refactor away redundency in inequality functions #181

Merged

Conversation

jitsedesmet
Copy link
Member

This PR aims to resolve #178 (working together but not dependent on each other is: #180)

@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same when pulling 8c896a7 on jitsedesmet:refactor/remove-ordering-redundency into 6c22f37 on comunica:master.

Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this already simplifies the code significantly, my question in #178 was not really about writing these functions in terms of each other, but if it would be possible to only depend on the orderTypes function. Do you think this makes sense? Or would this cause issues that I'm not thinking of?

@jitsedesmet
Copy link
Member Author

@rubensworks given the implementation in #180 is what you want for Ordering, I would keep making the regular functions call the regular functions since it is more self-contained.
Also, if we would want to call ordering, we would not want the fall through case as mentioned in a commend I made (and later resolved in favor of this PR)

@rubensworks
Copy link
Member

Also, if we would want to call ordering, we would not want the fall through case as mentioned in #180 (comment)

Ok, then we can change that I guess? That's the same reason why I added the strict parameter on orderTypes in d1e01c3.

@jitsedesmet
Copy link
Member Author

But why would you want this? I feel like this case is way better? You have all the logic of the regularfunctions in this one file?
Why would you want to go: RegularFunction -> Ordering -> RegularFunction when you can do RegularFunction -> RegularFunction?

@rubensworks
Copy link
Member

rubensworks commented Aug 7, 2023

But why would you want this? I feel like this case is way better? You have all the logic of the regularfunctions in this one file?
Why would you want to go: RegularFunction -> Ordering -> RegularFunction when you can do RegularFunction -> RegularFunction?

Not sure I understand you correctly.
What I suggest is to just delegate to the orderTypes function. AFAIK, orderTypes has no external references back to regular functions.

I just looked again at #180, and now I understand your comment, because you did add a back-reference to regular functions from type ordering there.

Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait with merging this one until #180 is merged.

@rubensworks
Copy link
Member

Closes #178

@rubensworks rubensworks merged commit 2e73d5c into comunica:master Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor out redundancies in RegularFunctions and Ordering
3 participants